Skip to content

refactor(conversations): separate search results and conversations#10051

Merged
ShGKme merged 2 commits intomasterfrom
refactor/left-sidebar-refactoring
Aug 18, 2023
Merged

refactor(conversations): separate search results and conversations#10051
ShGKme merged 2 commits intomasterfrom
refactor/left-sidebar-refactoring

Conversation

@ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Jul 24, 2023

☑️ Resolves

LeftSidebar refactoring

🖼️ Screenshots

No visual changes

🚧 Tasks

  • Fix "// TODO": instead of mutating the getter result as computed side effect, move sorting conversations into conversationsList getter
  • Divide storing all conversations in the LeftSidebar into: "All conversations list" + "Filtered conversations list" + "Search result conversations list"
    • It allows to easily add v-show (KeepAlive) or VirtualScrolling in future
  • Organize rendering in LeftSIdebar
    • Make conditions less labyrinthine =D
    • Remove duplication in conditions (the "no search result" part)

🏁 Checklist

@ShGKme ShGKme added this to the 💜 Next Major (28) milestone Jul 24, 2023
@ShGKme ShGKme requested review from Antreesy and DorraJaouad July 24, 2023 17:26
@ShGKme ShGKme self-assigned this Jul 24, 2023
Antreesy
Antreesy reviewed Jul 24, 2023
View reviewed changes
@ShGKme ShGKme force-pushed the refactor/left-sidebar-refactoring branch from a80f236 to 8a52bb0 Compare July 25, 2023 08:53
Antreesy
Antreesy approved these changes Jul 25, 2023
View reviewed changes
Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking, just a couple of remarks to improve

Antreesy
Antreesy reviewed Jul 25, 2023
View reviewed changes
@Antreesy Antreesy mentioned this pull request Jul 28, 2023
Open
@ShGKme ShGKme force-pushed the refactor/left-sidebar-refactoring branch 2 times, most recently from ca9303c to 4373318 Compare August 18, 2023 07:05
@ShGKme
Copy link
Contributor Author

ShGKme commented Aug 18, 2023

  • Updated according to the review
  • Rebased onto master with many LeftSidebar updates
  • Stylelint styles, there were wrong tabulation

@ShGKme ShGKme requested a review from Antreesy August 18, 2023 07:18
@ShGKme ShGKme marked this pull request as ready for review August 18, 2023 07:18
Antreesy
Antreesy approved these changes Aug 18, 2023
View reviewed changes
Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still works 🦭

@ShGKme ShGKme mentioned this pull request Aug 18, 2023
Merged
8 tasks
@nickvergessen
Copy link
Member

nickvergessen commented Aug 18, 2023

Conflicting files
src/components/LeftSidebar/LeftSidebar.vue

@ShGKme ShGKme force-pushed the refactor/left-sidebar-refactoring branch from 4373318 to 7f27d26 Compare August 18, 2023 11:46
@ShGKme
Copy link
Contributor Author

ShGKme commented Aug 18, 2023

Rebased onto master

@ShGKme ShGKme enabled auto-merge August 18, 2023 11:49
@ShGKme
Copy link
Contributor Author

ShGKme commented Aug 18, 2023

/backport to stable27

@ShGKme ShGKme disabled auto-merge August 18, 2023 12:39
@ShGKme
Copy link
Contributor Author

ShGKme commented Aug 18, 2023

There is a regression.

On the first load from a conversation page, before there was the current conversation in the list together with loading. Now there is no conversation before loading.

Actually, I don't know if it was intended before :D

@Antreesy @DorraJaouad @marcoambrosini @nickvergessen What do you think?

Before After
image image

@Antreesy
Copy link
Contributor

Antreesy commented Aug 18, 2023

Actually, I don't know if it was intended before :D

Probably not, as when you join a room, you fetch it separately from others. That's why it ended up alone in the store and in the list
Don't think, that it will be an issue with cached conversations, as we have a starting point already

@ShGKme
Copy link
Contributor Author

ShGKme commented Aug 18, 2023

Don't think, that it will be an issue with cached conversations, as we have a starting point already

The cache works only after the first open :D

@marcoambrosini
Copy link
Member

marcoambrosini commented Aug 18, 2023

before there was the current conversation in the list together with loading

I think it's a nice to have if in the content there's that conversation opened

@nickvergessen
Copy link
Member

nickvergessen commented Aug 18, 2023

Both behaviours are fine to me, so whatever is better to do/maintain

@ShGKme
Copy link
Contributor Author

ShGKme commented Aug 18, 2023

I think it's a nice to have if in the content there's that conversation opened

I checked that it is possible to make it work with both this loading and virtual scrolling.

Though, it is noticeable only after re-login when the new cache feature doesn't work, I decided to fix it.

So no regression now

@ShGKme ShGKme requested a review from Antreesy August 18, 2023 15:41
ShGKme added 2 commits August 18, 2023 18:09
@ShGKme
refactor(conversations): separate search results and conversations
ca42625 
- Fix "// TODO": instead of mutating the getter result as computed side effect, move sorting conversations into conversationsList getter
- Divide storing all conversations in the LeftSidebar into: "All conversations list" + "Filtered conversations list" + "Search result conversations list"
  It allows to easily add v-show (KeepAlive) or VirtualScrolling in future
- Organize rendering in LeftSIdebar and reduce duplications

Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@ShGKme
chore: lint styles
25d200e 
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@ShGKme ShGKme force-pushed the refactor/left-sidebar-refactoring branch from ceb5c26 to 25d200e Compare August 18, 2023 16:10
@ShGKme
Copy link
Contributor Author

ShGKme commented Aug 18, 2023

Rebased onto master and fixup.

@ShGKme ShGKme enabled auto-merge August 18, 2023 16:10
@ShGKme
Copy link
Contributor Author

ShGKme commented Aug 18, 2023

/backport to stable27

@ShGKme ShGKme merged commit 0cd7d45 into master Aug 18, 2023
@ShGKme ShGKme deleted the refactor/left-sidebar-refactoring branch August 18, 2023 17:52
@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Aug 18, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Antreesy Antreesy Antreesy approved these changes

@DorraJaouad DorraJaouad Awaiting requested review from DorraJaouad

Assignees

@ShGKme ShGKme

Labels

None yet

Projects

None yet

Milestone

v18.0.0-beta.1

Development

Successfully merging this pull request may close these issues.

4 participants

@ShGKme @nickvergessen @Antreesy @marcoambrosini